-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add metrics of kube_pod_status_ready_time and kube_pod_status_containers_ready_time redux #1938
Conversation
Welcome @ryanrolds! |
Holding in favor of #1837 for now. /hold |
Co-authored-by: Szymon Grzemski <sz.grzemski@gmail.com> Signed-off-by: Lan Liang <gcslyp@gmail.com>
3a52493
to
0e4e396
Compare
/triage accepted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two small requests, then we should be able to merge it.
for _, c := range p.Status.Conditions { | ||
if c.Type == v1.ContainersReady { | ||
ms = append(ms, &metric.Metric{ | ||
LabelKeys: []string{}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this require Label Keys identifying the container?
Similar to createPodContainerInfoFamilyGenerator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is it's not that a specific container is ready, but all the containers are ready. If we emitting a metric about the status of individual containers, I would add container id so it could be joined to kube_pod_container_info
. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you're right! I was thinking it's using something in .status.containerStatuses which would be per container. No need to change anything then. :)
- lastProbeTime: null
lastTransitionTime: "2022-11-18T18:50:53Z"
status: "True"
type: Ready
- lastProbeTime: null
lastTransitionTime: "2022-11-18T18:50:53Z"
status: "True"
type: ContainersReady
/hold cancel Thanks @liangyuanpeng and @ryanrolds for your contribution here! |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mrueg, ryanrolds The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Thanks for this. Very relevant to me currently. @mrueg may I ask whether this will make it to 2.8 ? And if yes, do you maybe have a rough idea when that could be published ? I'm not familiar with the release process here 😇 |
What this PR does / why we need it:
Base PR #1482
Ref: Adds kube_pod_status_ready_time and kube_pod_status_containers_ready_time metrics to get the information provided by Pod Lifecycle's PodConditions array.
Maybe we should support param to open this feature or not, juse like feature-gates.
How does this change affect the cardinality of KSM: (increases, decreases or does not change cardinality)
increases metrics number that Pod number * 2
Which issue(s) this PR fixes (optional, in fixes #(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1465
Co-authored-by: Szymon Grzemski sz.grzemski@gmail.com
Signed-off-by: Lan Liang gcslyp@gmail.com